Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add drop local user operation endpoint #610

Merged
merged 1 commit into from
Feb 21, 2024
Merged

feat: add drop local user operation endpoint #610

merged 1 commit into from
Feb 21, 2024

Conversation

dancoombs
Copy link
Collaborator

@dancoombs dancoombs commented Feb 17, 2024 •
edited
Loading

Closes #567

Proposed Changes

  • Adds rundler_dropLocalUserOperation rpc endpoint
  • Requirements:
    • The user operation must contain a sender/nonce pair this is present in the local mempool.
    • The user operation must pass entrypoint.simulateValidation. I.e. it must have a valid signature and verificationGasLimit
    • The user operation must have zero values for: preVerificationGas, callGasLimit, calldata, and maxFeePerGas
    • The user operation must be in the pool for at least 10 blocks before getting dropped.

@codecov Codecov
Copy link

codecov bot commented Feb 17, 2024 •
edited
Loading

Codecov Report

Attention: 180 lines in your changes are missing coverage. Please review.

Comparison is base (20b725f) 57.86% compared to head (1e72e7d) 57.59%.

Additional details and impacted files

Impacted file tree graph

Files Coverage Δ
crates/pool/src/mempool/error.rs 76.19% <ø> (ø)
crates/pool/src/mempool/mod.rs 87.50% <ø> (ø)
crates/pool/src/mempool/pool.rs 95.65% <100.00%> (+0.16%) ⬆️
crates/pool/src/server/mod.rs 25.00% <ø> (ø)
crates/provider/src/traits/entry_point.rs 50.00% <ø> (ø)
crates/rpc/src/eth/mod.rs 0.00% <ø> (ø)
crates/sim/src/simulation/simulation.rs 78.11% <ø> (ø)
crates/types/src/validation_results.rs 57.53% <ø> (ø)
bin/rundler/src/cli/node/mod.rs 0.00% <0.00%> (ø)
crates/pool/src/mempool/uo_pool.rs 92.65% <98.86%> (+0.45%) ⬆️
... and 13 more
Flag Coverage Δ
unit-tests 57.59% <39.18%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
rundler binary 0.00% <0.00%> (ø)
builder 51.30% <ø> (ø)
dev 0.00% <ø> (ø)
pool 64.62% <57.14%> (-0.22%) ⬇️
provider 0.87% <0.00%> (-0.04%) ⬇️
rpc 34.70% <0.00%> (-1.52%) ⬇️
sim 84.64% <0.00%> (+0.48%) ⬆️
tasks ∅ <ø> (∅)
types 86.82% <ø> (-4.45%) ⬇️
utils 14.96% <ø> (ø)

denniswon
denniswon reviewed Feb 18, 2024
View reviewed changes
Copy link

@denniswon denniswon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So uo hash is not needed for this api? What's the difference between uo hash bs uo Id (sender + nonce) for serving uniqueness?

Also after canceling the uo with nonce K, should nonce still be a new nonce value? (K+1) or K can still be the nonce of the next uo?

crates/rpc/src/rundler.rs

/// Drops a user operation from the local mempool.
///
/// Requirements:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. So it costs gas for verification to cancel a UO. Why the 3rd requirement for this end point instead of just ignoring? Just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't cost any gas on chain, but the UO must contain a high enough gas limit such that we can simulate it. This value is set by the user because its part of the UO that is signed.

crates/rpc/src/rundler.rs
Comment on lines +121 to +138
if uo.pre_verification_gas != U256::zero()
|| uo.call_gas_limit != U256::zero()
|| uo.call_data.len() != 0
|| uo.max_fee_per_gas != U256::zero()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these requirements exist for simulate uo operation?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean simulation verification

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for these requirements is to ensure that the signed UO is not executable onchain. That is, it should have 0 gas fees associated with it. The most important one here is max_fee_per_gas == 0, as that would cause the entrypoint to reject the UO. We could reduce this requirements set to just that.

Simulation verification when a normal UO is submitted doesn't have this requirement because the UO is meant to be included onchain.

@dancoombs
Copy link
Collaborator Author

So uo hash is not needed for this api? What's the difference between uo hash bs uo Id (sender + nonce) for serving uniqueness?

There is no API for a sender to lookup pending UOs. So if they find that they are stuck, i.e. getting replacement underpriced consistently, they can issue a cancel just knowing their address and their nonce. No need to also lookup a hash.

Also after canceling the uo with nonce K, should nonce still be a new nonce value? (K+1) or K can still be the nonce of the next uo?

K is still the nonce of the next UO, since nothing happened onchain.

@dancoombs
Copy link
Collaborator Author

This is missing the 10 block delay that we discussed in the ticket to prevent spam, need to add that before merge.

@dancoombs
Copy link
Collaborator Author

Also worth noting that this isn't a perfect drop mechanism. Its possible that the UO has already been bundled (or sent on the P2P network) before the drop.

@dancoombs dancoombs force-pushed the danc/balances-rpc branch 5 times, most recently from a727ed8 to c635e0a Compare February 21, 2024 13:02
@dancoombs dancoombs force-pushed the danc/balances-rpc branch 2 times, most recently from 5f327b9 to 85504b4 Compare February 21, 2024 15:29
Base automatically changed from danc/balances-rpc to main February 21, 2024 15:32
@dancoombs dancoombs marked this pull request as ready for review February 21, 2024 15:33
0xfourzerofour
0xfourzerofour approved these changes Feb 21, 2024
View reviewed changes
Copy link
Collaborator

@0xfourzerofour 0xfourzerofour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a nit

crates/pool/src/mempool/mod.rs Outdated Show resolved Hide resolved
crates/provider/src/ethers/entry_point.rs
@@ -46,21 +46,38 @@ where
self.deref().address()
}

async fn simulate_validation(
async fn get_simulate_validation_call(
&self,
user_op: UserOperation,
max_validation_gas: u64,
) -> anyhow::Result<TypedTransaction> {
let pvg = user_op.pre_verification_gas;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a comment but having spaces between distinct code blocks is very handy for vim movements rip

@dancoombs dancoombs merged commit 44fae8a into main Feb 21, 2024
7 checks passed
@dancoombs dancoombs deleted the danc/drop-rpc branch February 21, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denniswon denniswon denniswon left review comments

@0xfourzerofour 0xfourzerofour 0xfourzerofour approved these changes

@dphilipson dphilipson Awaiting requested review from dphilipson dphilipson is a code owner

@alex-miao alex-miao Awaiting requested review from alex-miao alex-miao is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the ability to drop a user operation in Rundler's local mempool
3 participants
@dancoombs @denniswon @0xfourzerofour